Change history time to be equal to the middle of the time bounds #2838
Conversation
...and other mods that I'm preserving from closed PR ESCOMP#2019, such as - changes to long_names and - treating avgflag as a tape (not field) trait for 'I' and 'L' tapes
…e_mid_of_time_bounds
slevis-lmwg
added
enhancement
|
I submitted this manual test to confirm that the committed modifications work as intended: |
|
The previous test completed its 1 month and the monthly output looked good, but there were annual history files that I could not tell. So I started another test (default is Ly1, but I changed to Ly2) and I added hist_avgflag_pertape(6) = 'I' to see what happens: PASS |
|
I updated the submodules to point to ESCOMP/MOSART#106 and ESCOMP/RTM#39 and submitted the three corresponding test-suites: All the cases that differ from the baseline, differ only in the time variable. UPDATE |
slevis-lmwg
added
the
external
slevis-lmwg
added
PR status: ready
time in hist now equals the middle of the time_bounds MOSART equivalent to CTSM work done in ESCOMP/CTSM#2838 Answers change only for the time variable. slevis resolved conflicts: src/riverroute/RtmHistFile.F90 src/riverroute/RtmTimeManager.F90
time in hist now equals the middle of the time_bounds RTM equivalent to CTSM work done in ESCOMP/CTSM#2838 Answers change only for the time variable.
|
@ekluzek review and approval of this PR should take 5 minutes, as it looks the same as the corresponding RTM and MOSART PRs that you reviewed/approved. Thanks :-) |
|
TODOs left for me:
|
|
izumi testing derecho testing Also I'm getting diffs in the cpl and mosart output of two tests BUT both are 3-yr tests: |
|
The diffs of the two tests above seem vaguely related to the earlier update to mosart1.1.02 (ESCOMP/MOSART#94), but Adrianna's test passed just fine pointing to mosart1.1.02. So I will try the two tests pointing to mosart1.1.02 and mosart1.1.03:
The same test from ctsm5.3.012: PASS DID THESE TESTS EXIST WHEN I LAST RAN aux_clm? Yes (ctsm5.3.009). So now I checked out 1e81456 from above, pointed to mosart1.1.04/rtm1_0_82, and submitted: |
|
I brainstormed for a bit with @billsacks and Bill pointed out/suggested:
|
|
@billsacks I mentioned to you a vague memory I had of an issue that could relate to these diffs, and it is this one: |
Ah, yes. But I don't think that should be the issue with these tests, right? |
|
Right, I don't think so. I think I have now found that the problem starts with the introduction of mosart1.1.03. I see this in a new test today and in my testing from yesterday (somehow I missed the sign when I looked originally). First I will confirm beyond doubt and then I will try bisecting mosart1.1.03 to find the culprit. |
Looking at ESCOMP/MOSART#70, I have converged on two commits: I have confirmed that the two lines that we removed caused the diffs. My first guess: Keep the if-statement but need changes elsewhere to make the if-statement be true as suggested in ESCOMP/MOSART#103 |
|
Trying a case with cism NOT active and the if-statement still commented out (as in my last test) Submitted |
…ds-ssr Handle "instantaneous files" in RXCROPMATURITY analysis script
|
Rerunning this test to generate a baseline, but stuck in the SHAREDLIB_BUILD phase, so I will kill it and try again next week. Besides, I will need to generate a ctsm5.3.014 baseline, so rerunning right now is redundant: |
|
I'm wondering if the "treat a file as instantaneous if its first variable is" might be premature to bring in here instead of #2445. Specifically, I think the |
@olyson what do you think about @samsrabin's comment? Most concerning to me would be any vulnerability in the land diagnostic package. |
|
I don't think the land diagnostics package uses time_bounds. But I may not understand the issue here. Seems like we could do a short simulation using this branch once stable and see if there are any problems? Also should check ILAMB. So maybe an I2000 case for a test. |
|
@olyson Okay, good that the land diagnostics package probably doesn't use it, but yes would be nice to check. However, I'm thinking that other people's scripts might rely on the presence of |
|
And actually, this comment goes for the "exact middle" vs. "end of" change as well. It seems arbitrary (and against the "principle of least astonishment") that the first variable in the |
|
A bonus from what I'm proposing: Always (a) including |
|
The standard diagnostics package doesn't use time_bounds as far as I can tell. ILAMB may, it just needs to be tested. Always including time_bounds sounds fine to me. |
|
time_bounds is an expected part of the CF convention, so I do endorse using it for anything with time. That's likely why some tools might expect it to be there. For instantaneous it should likely be the time bounds of the time-step that was output. So the previous time-step time first to the ending time-step time for the endpoint. You could have both being the same ending time-step, but that doesn't show that it is a model with a finite time-step. Here's information on the CF Convention attributes. Look up "bounds"... |
|
@ekluzek That link contains the following text, which implies that instantaneous files should actually not have
|
|
Good point @samsrabin. That convinces me we should remove it for I fields then. That would then be in line with the convention. We don't similarly report on the grid cell bounds either (which could be done for 2D grids, but would be harder for unstructured grids), so we shouldn't for Instantaneous time fields either. |
|
Earlier discussion led to the conclusion that we should not have time_bounds in instantaneous files (though CAM's mistakenly still does): ESCOMP/CAM#1166 |
|
I had a quick meeting with @samsrabin 40 minutes ago: Oh, good, looks as though we're removing time_bounds from instantaneous tapes, as originally planned :-) Ok, so I'm testing the alternate if-statement with aux_clm right now and then I will push it to the PR.
How I'm checking whether diffs are expected: OK aux_clm except failure reported in the following post. |
|
@samsrabin back to this test |
|
@samsrabin I'm thinking please hold regarding my last post for now, because in my aux_clm tests with the two subsequent PRs, which included this one, the test passed. |
samsrabin
removed
the
PR status: ready
|
Note to @ekluzek
Note to self: |
Description of changes
This PR subsets the scope of issue #1059 and PR #2445 as a result of the October 2024 conversation in #2445.
This PR changes history time to be equal to the middle of the time bounds.
This PR does not put instantaneous fields on their own separate history files.
I will also bring submodule changes from ESCOMP/MOSART#106 (was ESCOMP/MOSART#69) and ESCOMP/RTM#39.
Specific notes
Contributors other than yourself, if any:
Are answers expected to change (and if so in what way)?
No.
Does this create a need to change or add documentation? Did you do so?
Maybe. No.
Testing performed, if any:
Plan to run aux_clm, mosart, rtm test-suites.